Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix(eos_cli_config_gen,eos_designs): Dont configure access group on interface when access group is defined on session level #4565

Merged
merged 35 commits into from
Oct 17, 2024
Merged

Fix(eos_cli_config_gen,eos_designs): Dont configure access group on interface when access group is defined on session level #4565

merged 35 commits into from
Oct 17, 2024

Conversation

laxmikantchintakindi
Copy link
Contributor

@laxmikantchintakindi laxmikantchintakindi commented Oct 9, 2024 •
edited
Loading

Change Summary

Dont configure access group to interface when access group is defined on session level

Related Issue(s)

Fixes #4524

Component(s) name

arista.avd.eos_cli_config_gen
arista.avd.eos_designs

Proposed changes

Dont configure access group to interface when access group is defined on session level

s1-leaf1(config)#monitor session m1 ip access-group ipv4ACL
! Access list ipv4ACL not configured. Assigning anyway.
s1-leaf1(config)#monitor session m1 source ethernet 1 ipv6 access-group abcd
! Source Ethernet1 in Session m1 is applied with access list ipv4ACL with type ip instead of access list abcd with type ipv6. Only one access list type per session is supported.

How to test

Checklist

User Checklist

  • N/A

Repository Checklist

  • My code has been rebased from devel before I start
  • I have read the CONTRIBUTING document.
  • My change requires a change to the documentation and documentation have been updated accordingly.
  • I have updated molecule CI testing accordingly. (check the box if not applicable)

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 9, 2024

Review docs on Read the Docs

To test this pull request:

# Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-4565
# Activate the virtual environment
source test-avd-pr-4565/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/laxmikantchintakindi/avd.git@bug/monitor-session#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/laxmikantchintakindi/avd.git#/ansible_collections/arista/avd/,bug/monitor-session --force
# Optional: Install AVD examples
cd test-avd-pr-4565
ansible-playbook arista.avd.install_examples

@github-actions github-actions bot added the state: CI Updated CI scenario have been updated in the PR label Oct 9, 2024
@laxmikantchintakindi laxmikantchintakindi changed the title Fix(eos_cli_config_gen): Dont configure access group to interface when access group is defined on session level Fix(eos_cli_config_gen): Dont configure access group on interface when access group is defined on session level Oct 9, 2024
@laxmikantchintakindi
Copy link
Contributor Author

Can I update eos_designs for monitor_sesion with this logic?

@ClausHolbechArista
Copy link
Contributor

Can I update eos_designs for monitor_sesion with this logic?

Sure, then update the scope on PR title to be both roles.

@laxmikantchintakindi laxmikantchintakindi changed the title Fix(eos_cli_config_gen): Dont configure access group on interface when access group is defined on session level Fix(eos_cli_config_gen, eos_designs): Dont configure access group on interface when access group is defined on session level Oct 9, 2024
@ClausHolbechArista ClausHolbechArista changed the title Fix(eos_cli_config_gen, eos_designs): Dont configure access group on interface when access group is defined on session level Fix(eos_cli_config_gen,eos_designs): Dont configure access group on interface when access group is defined on session level Oct 9, 2024
@laxmikantchintakindi
Copy link
Contributor Author

Can I update eos_designs for monitor_sesion with this logic?

Sure, then update the scope on PR title to be both roles.

Fixed.

@gmuloc gmuloc added this to the v5.0.0 milestone Oct 10, 2024
@Shivani-gslab Shivani-gslab requested a review from gmuloc October 14, 2024 11:19
@Shivani-gslab Shivani-gslab marked this pull request as ready for review October 14, 2024 11:22
@alexeygorbunov
Copy link
Contributor

alexeygorbunov commented Oct 16, 2024 •
edited
Loading

Would it be useful to have a 3rd negative test case where part of the config is coming from connected_endpoints and other part is coming from network_ports?

loopback_ipv4_pool: 192.168.1.0/24

type: l2leaf
l2leaf:
  defaults:
  nodes:
    - name: connected-endpoints-monitor-session-connected-endpoint-network-port-acl

servers:
  - name: INDIVIDUAL_1
    adapters:
      - switches: [connected-endpoints-monitor-session-connected-endpoint-network-port-acl]
        switch_ports: [Ethernet14]
        description: Monitor Ethernet 14
        monitor_sessions:
          - name: DMF
            role: source
            session_settings:
              access_group:
                type: ip
                name: acl1

network_ports:
  - switches:
      - connected-endpoints-monitor-session-connected-endpoint-network-port-acl
    switch_ports:
      - Ethernet14
    description: PC
    monitor_sessions:
      - name: DMF
        role: source
        source_settings:
          access_group:
            type: ip
            name: acl2

expected_error_message: >-
  Cannot set an ACL for both `session_settings` and `source_settings` under the monitor session 'DMF'
  for network_ports[0].

TASK [arista.avd.eos_designs : Generate device configuration in structured format] ***
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: pyavd._errors.AristaAvdInvalidInputsError: Cannot set an ACL for both `session_settings` and `source_settings` under the monitor session 'DMF' for network_ports[0].
fatal: [connected-endpoints-monitor-session-connected-endpoint-network-port-acl -> localhost]: FAILED! => {"changed": false, "msg": "Cannot set an ACL for both `session_settings` and `source_settings` under the monitor session 'DMF' for network_ports[0]."}

TASK [Assert eos_designs failed with the expected error message] ***************
ok: [connected-endpoints-monitor-session-connected-endpoint-network-port-acl] => {
    "changed": false,
    "msg": "All assertions passed"
}

@ClausHolbechArista
Copy link
Contributor

Would it be useful to have a 3rd negative test case where part of the config is coming from connected_endpoints and other part is coming from network_ports?

To not add too many tests for corner cases, this scenario could be the only one, since it would work the same no matter where the config is coming from.

Laxmikant Chintakindi added 2 commits October 17, 2024 12:03
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@ClausHolbechArista ClausHolbechArista merged commit 40451d8 into aristanetworks:devel Oct 17, 2024
43 checks passed
@ClausHolbechArista
Copy link
Contributor

@alexeygorbunov remember to use request changes on the reviews if you want something changed, since it will block merging the PR. I forgot about the updated test, but it can go in a separate "Test" PR.

gmuloc pushed a commit to gmuloc/avd that referenced this pull request Oct 29, 2024
@laxmikantchintakindi @gmuloc
Fix(eos_cli_config_gen,eos_designs): Dont configure access group on i…
1dd6003 
…nterface when access group is defined on session level (aristanetworks#4565)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexeygorbunov alexeygorbunov alexeygorbunov left review comments

@Vibhu-gslab Vibhu-gslab Vibhu-gslab left review comments

@MaheshGSLAB MaheshGSLAB MaheshGSLAB approved these changes

@ClausHolbechArista ClausHolbechArista ClausHolbechArista approved these changes

@gmuloc gmuloc Awaiting requested review from gmuloc

Assignees

@laxmikantchintakindi laxmikantchintakindi

Labels
rn: Fix(eos_cli_config_gen|eos_designs) role: eos_designs issue related to eos_designs role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated
Projects
None yet
Milestone
v5.0.0
Development

Successfully merging this pull request may close these issues.

Bug(eos_cli_config_gen, eos_designs): Monitor-session needs correction
6 participants
@laxmikantchintakindi @ClausHolbechArista @alexeygorbunov @gmuloc @Vibhu-gslab @MaheshGSLAB